add FieldName API #79

sivchari · 2025-05-02T11:40:32Z

Add FieldName API to reduce the redundant code.

FieldName returns the name of filed, then if the field name is empty that means embedded field, return ident.Name instead.

/cleanup

JoelSpeed

Have you checked through other rules to see if there are other places that could use this util?

JoelSpeed · 2025-05-02T13:00:21Z

pkg/analysis/jsontags/analyzer.go

+	prefix := "embedded field %s"
 	if len(field.Names) > 0 && field.Names[0] != nil {
-		prefix = fmt.Sprintf("field %s", field.Names[0].Name)
-	} else if ident, ok := field.Type.(*ast.Ident); ok {
-		prefix = fmt.Sprintf("embedded field %s", ident.Name)
+		prefix = "field %s"
 	}


Nit, embedded fields are the exception, not the rule, so I would invert this

prefix := "field %s" if len(field.Names) == 0 || field.Names[0] == nil { prefix = "embedded field %s" }

Oops, fixed.

JoelSpeed · 2025-05-02T13:01:15Z

pkg/analysis/jsontags/analyzer.go

 	}

 	if tagInfo.Missing {
-		pass.Reportf(field.Pos(), "%s is missing json tag", prefix)
+		pass.Reportf(field.Pos(), "%s is missing json tag", fmt.Sprintf(prefix, utils.FieldName(field)))


Probably better to call the FieldName once/twice when we are setting up the prefix, than three times down here

JoelSpeed · 2025-05-02T13:01:40Z

pkg/analysis/utils/utils.go

+// FieldName returns the name of the field. If the field has a name, it returns that name.
+// If the field is embedded and it can be converted to an identifier, it returns the name of the identifier.
+func FieldName(field *ast.Field) string {
+	if len(field.Names) > 0 {


Check as well that Names[0] isn't nil

pkg/analysis/utils/utils.go

yongruilin · 2025-05-06T21:19:20Z

pkg/analysis/utils/utils.go

@@ -77,6 +77,21 @@ func LookupTypeSpec(pass *analysis.Pass, ident *ast.Ident) (*ast.TypeSpec, bool)
 	return nil, false
 }

+// FieldName returns the name of the field. If the field has a name, it returns that name.
+// If the field is embedded and it can be converted to an identifier, it returns the name of the identifier.
+func FieldName(field *ast.Field) string {


What's the expected fieldName for pointer embedded fields? e.g.

*StringPtrField `json:"stringPtrField"`

Under current implementation, it would be "". Should the output be *StringPtrField?

Good catch! Thanks, fixed.

Can we create some test cases for this function that demonstrate it's functionality? Make sure if we make updates here in the future we don't forget why we are handling StarExpr?

Added test cases

Signed-off-by: sivchari <shibuuuu5@gmail.com>

JoelSpeed · 2025-05-08T11:09:43Z

pkg/analysis/utils/utils_test.go

+func TestFieldName(t *testing.T) {
+	t.Parallel()
+
+	cases := map[string]struct {
+		field *ast.Field
+		want  string
+	}{
+		"field has Names": {
+			field: &ast.Field{
+				Names: []*ast.Ident{
+					{
+						Name: "foo",
+					},
+				},
+			},
+			want: "foo",
+		},
+		"filed has no Names, but is an Ident": {
+			field: &ast.Field{
+				Type: &ast.Ident{
+					Name: "foo",
+				},
+			},
+			want: "foo",
+		},
+		"field has no Names, but is a StarExpr with an Ident": {
+			field: &ast.Field{
+				Type: &ast.StarExpr{
+					X: &ast.Ident{
+						Name: "foo",
+					},
+				},
+			},
+			want: "foo",
+		},
+		"field has no Names, and is not an Ident or StarExpr": {
+			field: &ast.Field{
+				Type: &ast.ArrayType{
+					Elt: &ast.Ident{
+						Name: "foo",
+					},
+				},
+			},
+			want: "",
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			got := utils.FieldName(tc.field)
+			if got != tc.want {
+				t.Errorf("got %q, want %q", got, tc.want)
+			}
+		})
+	}
+}


Can we use ginkgo? Since other tests are using ginkgo already. The example below would show how this would look

func TestUtils(t *testing.T) { RegisterFailHandler(Fail) RunSpecs(t, "Utils") } var _ = Describe("FieldName", func() { type fieldNameTestCase struct { field *ast.Field want string } DescribeTable("Should extract the field name", func(tc fieldNameTestCase) { Expect(utils.FieldName(tc.field)).To(Equal(tc.want), "expected the extracted field name to match) }, Entry("field has Name", fieldNameTestCase{ field: &ast.Field{ Names: []*ast.Ident{ { Name: "foo", }, }, }, want: "foo", }), ... ) })

The actual TestUtils conventionally goes in a utils_suite_test.go file on its own

Okay, I rewrote the test case'

Signed-off-by: sivchari <shibuuuu5@gmail.com>

JoelSpeed · 2025-05-08T14:30:54Z

/approve
/lgtm

k8s-ci-robot · 2025-05-08T14:31:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed, sivchari

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JoelSpeed]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 2, 2025

k8s-ci-robot requested review from everettraven and jpbetz May 2, 2025 11:40

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 2, 2025

JoelSpeed reviewed May 2, 2025

View reviewed changes

sivchari force-pushed the add-astfield-util branch from 5b9b769 to 97ea957 Compare May 3, 2025 12:36

everettraven reviewed May 5, 2025

View reviewed changes

pkg/analysis/utils/utils.go Show resolved Hide resolved

yongruilin reviewed May 6, 2025

View reviewed changes

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 7, 2025

sivchari added 5 commits May 8, 2025 13:23

add FieldName API

6384e29

Signed-off-by: sivchari <shibuuuu5@gmail.com>

add nil check

ac3e5e2

Signed-off-by: sivchari <shibuuuu5@gmail.com>

add nil check

b2f3683

Signed-off-by: sivchari <shibuuuu5@gmail.com>

hadnle StartExpr

ec40bd7

Signed-off-by: sivchari <shibuuuu5@gmail.com>

add FieldName test case

90cf8be

Signed-off-by: sivchari <shibuuuu5@gmail.com>

sivchari force-pushed the add-astfield-util branch from 0d2b0d5 to 90cf8be Compare May 8, 2025 04:25

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 8, 2025

JoelSpeed reviewed May 8, 2025

View reviewed changes

use ginkgo

c17423e

Signed-off-by: sivchari <shibuuuu5@gmail.com>

k8s-ci-robot assigned JoelSpeed May 8, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 8, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 8, 2025

k8s-ci-robot merged commit 00a5c0f into kubernetes-sigs:main May 8, 2025
4 checks passed

add FieldName API #79

add FieldName API #79

Conversation

sivchari commented May 2, 2025

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoelSpeed commented May 8, 2025

Uh oh!

k8s-ci-robot commented May 8, 2025

Uh oh!

Uh oh!

Uh oh!